[stdlib] Use addressor for Dictionary's subscript(_:default:) #12752
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hamishknight
commented
Nov 3, 2017
| @@ -25,6 +25,31 @@ func equalsUnordered<T : Comparable>(_ lhs: [T], _ rhs: [T]) -> Bool { | |||
| return lhs.sorted().elementsEqual(rhs.sorted()) | |||
| } | |||
|
|
|||
| // A COW wrapper type that holds an Int. | |||
There was a problem hiding this comment.
Is this the right place to define this, and is there an already existing mock COW wrapper I could use here?
There was a problem hiding this comment.
It's perfectly fine to add this here!
|
@swift-ci Please smoke test |
|
@swift-ci Please benchmark |
|
@hamishknight This is super interesting! It looks like this might also solve an issue where using a class instance as the default doesn't actually assign: var d: [String: UIView] = [:]
d["foo", default: UIView()].addSubview(UIView())
print(d["foo"] as Any)Also, re this bit, the CI has been updated so it can handle new benchmarks in this same PR:
|
|
@natecook1000 Thanks! I don't believe it'll solve the problem you mention, as the compiler will still just call the getter directly (as it's dealing with a reference type), therefore not inserting the default value. I believe that'd have to be a compiler-level change to make that work (well not unless the getter is changed to always attempt to insert the default value; but that goes contrary to the semantics you set out in the evolution proposal that introduced the subscript).
Awesome – I'll get on with that then :) |
hamishknight
changed the title
|
@natecook1000 I've gone ahead and added the benchmarks, could you please kick off the CI bot? (I'm not sure whether or not I'm able to). I think this PR is ready for review now :) |
|
@swift-ci Please smoke test |
|
@swift-ci Please benchmark |
1 similar comment
|
@swift-ci Please benchmark |
Build comment file:Optimized (O)Regression (4)
Improvement (15)
No Changes (315)
Added (4)
Unoptimized (Onone)Regression (9)
Improvement (47)
No Changes (278)
Added (4)
Hardware Overview |
Build comment file:Optimized (O)Regression (8)
Improvement (30)
No Changes (296)
Added (4)
Unoptimized (Onone)Regression (5)
Improvement (8)
No Changes (321)
Added (4)
Hardware Overview |
|
Some nice performance gains here! |
…gDefault:) The reasoning for this change is detailed in PR-13000.
hamishknight
force-pushed
the
dictionary-subscript-addressor
branch
from
6acee0d
to
76ea3a2
Compare
|
Just fixed the uniqueness check in Would it be best to rebase the commits into 1, or leave them as-is? Also, any chance of getting this moving soon? :) |
| @@ -2048,7 +2048,7 @@ public struct Dictionary<Key : Hashable, Value> : | |||
| } | |||
| mutableAddressWithNativeOwner { | |||
| let (_, address) = _variantBuffer | |||
| .pointerToValue(forKey: key, insertingDefault: defaultValue) | |||
| .pointerToValue(forKey: key, insertingDefault: defaultValue()) | |||
There was a problem hiding this comment.
Could we remove the @autoclosure attribute from the _VariantDictionaryBuffer methods instead? Without being able to see the signature for pointerToValue it really looks like defaultValue is getting called here.
There was a problem hiding this comment.
I didn't have any strong opinion on it at the time; but now you mention it, yup, I agree.
…ult:) methods Previously we used @autoclosure on the _VariantDictionaryBuffer methods and forwarded closures directly. However, this relied on an unintentional type-checker inconsistency, tracked by SR-5719.
hamishknight
force-pushed
the
dictionary-subscript-addressor
branch
from
a897f28
to
12416f9
Compare
|
Any news on this @moiseev? |
|
Oops. Sorry did not pay attention. |
|
The trouble is we don't have the law of exclusivity, at least not for all code, it's still opt-in. So if a user has it turned off we need to be very careful about exactly what the situation is in that case. If it's just odd but safe behaviors in case of situations that area already considered invalid that might be ok, unsafe/undefined behavior would not. cc @atrick / @rjmccall |
|
@airspeedswift This should fall under "odd but safe behavior" – the only difference to a user running code without exclusivity checks should be that during the window of mutation of a dictionary through Do we actually guarantee specified behaviour for breaking the law of exclusivity though? From what I could tell, the stance towards that was it mustn't introduce undefined behaviour, but the user should expect their program to be difficult to reason about (is this correct?). IMO, this change fits well with that. Although aren't exclusivity checks currently opt-out? |
|
Because we do not yet have guaranteed exclusivity, you need to use an owning addressor in order to ensure that nobody destroys or structurally modifies the buffer during the subscript access. But since you are using an owning addressor, I think this should be fine. |
lorentey
approved these changes
Dec 3, 2017
| // | ||
| // SR-6437 | ||
| let capacity = asNative.capacity | ||
| _ = ensureUniqueNativeBuffer(capacity) |
There was a problem hiding this comment.
Shouldn't we just call _nativePointerToValue(at:) here?
There was a problem hiding this comment.
Thanks @lorentey, I've gone ahead and made that change.
| @@ -25,6 +25,31 @@ func equalsUnordered<T : Comparable>(_ lhs: [T], _ rhs: [T]) -> Bool { | |||
| return lhs.sorted().elementsEqual(rhs.sorted()) | |||
| } | |||
|
|
|||
| // A COW wrapper type that holds an Int. | |||
There was a problem hiding this comment.
It's perfectly fine to add this here!
|
@swift-ci Please test |
|
Build failed |
…Key:insertingDefault:)
|
@swift-ci Please test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Given we now have the law of exclusivity, I believe we can use an addressor for
Dictionary'ssubscript(_:default:)by first inserting the default value if not already present for the given key, and handing back a pointer to the value in the buffer.This allows us to prevent the copying of a dictionary's value on mutation through this subscript. For example, we can now mutate an array value in-place:
The only cons that I can see to this change is that:
Using the subscript for plain assignment (
dict["foo", default: []] = ...) is now less efficient, as it'll attempt to insert the default value before immediately overwriting it with the new value (and needlessly evaluate the autoclosure'd default value if inserted). However, frankly using assignment with this subscript is pointless, as the default value has no effect.Code that runs without exclusivity checks will be able to see the default value already inserted while in the window of mutation. However I don't believe we guarantee any specified behaviour for breaking the law of exclusivity (is that right?).
I appreciate any and all feedback on this change; specifically whether or not I've missed any reason why we cannot/shouldn't do this.